Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Path naming fix #315

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Path naming fix #315

wants to merge 3 commits into from

Conversation

jrh03
Copy link
Contributor

@jrh03 jrh03 commented Aug 24, 2024

The previous body processing issue was simply to do with the config file (I figured after an hour or two). After deleting it everything began to redownload as expected.

The only other issue I had left was that some files stored in the ultraDocumentBody had them stored under an extra, unnecessary directory titled "ultraDocumentBody". These changes seem to fix that. This may not be the optimal implementation, however it seems simple and works for me.

Line 56, the handling of the ignoring in the write content method, may also not need the is_ultra flag, however after checking out the directory it seems to work well with it there, hence I haven't removed it.

@jrh03
Copy link
Contributor Author

jrh03 commented Aug 24, 2024

#314

Sorry, I haven't submitted a PR before. Also, the first commit that you may see in this PR was completely unnecessary as my issue was due to the config file.

@sanjacob
Copy link
Owner

sanjacob commented Aug 24, 2024 •
edited
Loading

Thanks for the PR. I understand that it might be confusing to have a directory named "ultraDocumentBody" instead of having the files in the parent directory.

However, there are two concerns I have.

  1. Name collision: what happens if within the parent directory there is already a file with that title. At least with api-only files titles are probably not duplicated. But I'm not sure the webdav body has that constraint.
  2. Is this title an established convention? How sure can we be that this title isn't different for other universities or versions of blackboard learn?

If these two are not an issue anymore then rather than manually checking if the content is ultra_body in the content base class, it's better to change this in the create_dir property of the content handler classes. I'm pretty sure right now I have set it as a classmethod, but this could easily depend on the content itself for Document. Thus, in the Document class, ultraDocumentBody content will not create a directory and will pass the same path it got passed to its content body links.

@sanjacob sanjacob added the on-hold This pr will be considered in the long term label Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
on-hold This pr will be considered in the long term
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jrh03 @sanjacob